Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transfer search parameters from page URL to jupyterlite #108

Merged
merged 7 commits into from
Sep 11, 2023

Conversation

brichet
Copy link
Contributor

@brichet brichet commented Sep 7, 2023

This PR adds a new directive to allow transfering some search params from the documentation URL to the jupyterlite URL.

Fixes #107

@martinRenou
Copy link
Member

Neat! Could you add an example of this in the docs?

@brichet brichet changed the title Transfert search parameters from page URL to jupyterlite Transfer search parameters from page URL to jupyterlite Sep 7, 2023
@brichet brichet marked this pull request as ready for review September 7, 2023 14:32
@@ -242,6 +257,10 @@ def run(self):
prompt = self.options.pop("prompt", False)
prompt_color = self.options.pop("prompt_color", None)

search_params = list(
Copy link
Member

@martinRenou martinRenou Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the search_param directive here for whitelisting the parameters that we want to transfer? Would there be an issue transferring all of them blindly?

Copy link
Contributor Author

@brichet brichet Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure to understand.
It's here to build a python list from the string, but yes we could probably transfer the whole string and directly build a javascript array from it.
Do you think of an other way ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is the following (removing the search_param directive and transferring all query parameteres to the iframe)

window.jupyterliteConcatSearchParams = (iframeSrc) => {
  const baseURL = window.location.origin;
  const iframeUrl = new URL(iframeSrc, baseURL);

  let pageParams = new URLSearchParams(window.location.search);

  pageParams.keys().forEach(param => {
    value = pageParams.get(param);
    if (value !== null) {
      iframeUrl.searchParams.append(param, value);
    }
  });

  return iframeUrl.toString().replace(baseURL, '');
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure it makes sense to forward all query parameters. But it would simplify the code and user API.

Copy link
Contributor Author

@brichet brichet Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thanks, I understand your first comment. Yes, the idea is to whitelist the parameters.

I am not sure it makes sense to forward all query parameters. But it would simplify the code and user API.

It's better to have at least the option (like a flag) to let user allow the transfer or not.
I don't know which is the best.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could add an option to transfer all the parameters(=all for example, since = can probably not be used as parameter name), or use only a boolean value to transfer all or none.

Do you have an opinion on it @martinRenou ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, we could have the search_params be either a list of strings (whitelist) or a boolean (True=transfer all, False=transfer none).

Also, I wonder if this could be implemented as an option for other directives, instead of a global search_params directive, like:

.. jupyterlite::
   :search_params: True

Or

.. replite::
   :search_params: ["theme", "code"]

This way it will be more granular

@martinRenou martinRenou added the enhancement New feature or request label Sep 11, 2023

The directive `search_params` allows to transfer some search parameters from the documentation URL to the Jupyterlite URL.\
Jupyterlite will then be able to fetch these parameters from its own URL.\
For example `:search_params: ["param1", "param2"]` will transfer the parameters *param1* and *param2*.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to provide a practical example that really applies to JupyterLite? Like "theme" for replite?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not, but I don't really know how it can be displayed to user.

The simplest solution I can find is to use something like

%%javascript
alert(window.location.search);

in a notebook, formatting a bit better the attributes.

Otherwise I didn't find a way to share value between javascript and python in a notebook, using xeus kernel.
It would be better to display the values in an output rather than using an alert.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's merge it as is for now!

jupyterlite_sphinx/jupyterlite_sphinx.py Outdated Show resolved Hide resolved
@martinRenou
Copy link
Member

@brichet let's get this in! Unless you'd like to push something else?

Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@martinRenou martinRenou merged commit d69ce1f into jupyterlite:main Sep 11, 2023
3 checks passed
@brichet brichet deleted the transfert_search_params branch September 11, 2023 11:00
@brichet
Copy link
Contributor Author

brichet commented Sep 11, 2023

Thanks @martinRenou for review

@brichet brichet mentioned this pull request Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transfert search parameters from Page URL to Jupyterlite URL
2 participants